Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

📦 Declare mas as a Swift Package #350

Merged
merged 33 commits into from
May 3, 2021
Merged

📦 Declare mas as a Swift Package #350

merged 33 commits into from
May 3, 2021

Conversation

chris-araman
Copy link
Contributor

@chris-araman chris-araman commented Apr 27, 2021

  • Removes dependence on carthage and make.
  • Moves sources to Sources and Tests as is the default in new Swift packages.
    • If others would prefer these remain as they were, we can instead specify the paths explicitly in Package.swift.
  • Declares Swift package dependencies in Xcode project and Package.swift.
  • Removes the Xcode project and workspace. (See below.)
  • swift build and swift test succeed.
  • Updates scripts to use swift build and swift test. These continue to build universal (fat, arm64 and x86_64) binaries for macOS 10.11 and later. In the future, we could build for one architecture or the other, and have platform-specific bottles as many other formulae do.

To Do:

  • Decide whether we want to remove the Xcode project and add swift package generate-xcodeproj to script/bootstrap instead. This would allow us to avoid maintaining multiple ways of building mas, and keeping duplicate dependency lists. The build scripts are still building the Xcode project instead of the Swift Package.
    It turns out that generate-xcodeproj is deprecated and unnecessary. Xcode can open the mas directory directly because it's now a Swift Package.
  • Decide if it's okay to link MasKit as a library into mas instead of deploying it as a framework. SPM does not yet support building frameworks, only libraries and executables.
    I'm moving forward with the assumption that treating this as an SPM library is fine moving forward. If Apple decides in the future to add support for frameworks, we can add a corresponding product or target to Package.swift. The mas binary can now be relocated (Allow running mas as portable or installing it  #326). The Homebrew bottles are now much smaller.
  • Decide if we want to keep our Info.plist. SPM does not have a supported way to process and inject these, though we could probably manage to write some .linkerSettings(.unsafeFlags(...)) hackery that would get the job done. It looks like we only inject copyright and version strings. The version command currently uses the injected version string. We could inject that in a different way.
    I've added logic to script/version to generate Sources/MasKit/Package.swift with a string constant Package.Version. The VersionCommand now emits this constant instead of CFBundleShortVersionString. The copyright still exists in LICENSE, though not in the compiled binary. Should I add a copyright command or emit the copyright info from help?
  • Resolve the GitHub Action test failures.
  • Fix the mas.pkg build.

Fixes #326.

Package.swift Show resolved Hide resolved
Package.swift Show resolved Hide resolved
Package.swift Show resolved Hide resolved
Package.swift Show resolved Hide resolved
Package.swift Show resolved Hide resolved
Package.swift Show resolved Hide resolved
Package.swift Show resolved Hide resolved
Package.swift Show resolved Hide resolved
Package.swift Show resolved Hide resolved
Package.swift Show resolved Hide resolved
@chris-araman chris-araman marked this pull request as draft April 27, 2021 04:16
phatblat
phatblat previously approved these changes Apr 28, 2021
@chris-araman chris-araman force-pushed the spm branch 3 times, most recently from 015847d to e8ca54c Compare April 29, 2021 01:52
@chris-araman
Copy link
Contributor Author

If we can find a new home for the current version number (somewhere that's easy to remember) then we can lose the Info.plist file.

The new home is in git tag. script/version will now fetch the latest git tag on the current branch, strip the v prefix, and generate Sources/MasKit/Package.swift using the tag as the Package.Version constant used by VersionCommand.

@rgoldberg
Copy link
Contributor

rgoldberg commented Apr 30, 2021

Will a new version be released once this & #352 are merged?

Some apps are not being upgraded by mas upgrade in 1.8.1. I want to investigate & possibly fix these issues, but it doesn't make sense to do so until your recent changes have been merged into master, and preferably released, too.

@chris-araman
Copy link
Contributor Author

@rgoldberg, I hope so. I have some other proposals (#346 (comment), #344 (comment), #309 (comment)) in the works, but none of them should prevent this PR and the previous ones from being released.

@rgoldberg
Copy link
Contributor

Cool. Best to release everything that's almost ready ASAP, instead of delaying those things for new PRs.

@chris-araman
Copy link
Contributor Author

@rgoldberg, you're a member of the mas-cli organization. Are you available to review this PR?

@chris-araman chris-araman requested a review from rgoldberg May 2, 2021 18:08
@phatblat
Copy link
Member

phatblat commented May 2, 2021

@rgoldberg @chris-araman FYI the 3 of us are the only active members of the mas-cli org. I would like to get a release out soon and think we're still looking at a patch release (1.8.2) with bug fixes and (many great) project improvements.

One piece that I haven't figured out yet is what to do about the hosting for the bottle files for our custom homebrew-tap since Bintray has been shutdown. This shouldn't block the release.

@rgoldberg
Copy link
Contributor

rgoldberg commented May 2, 2021

Unfortunately, I know almost nothing about Xcode, Swift, Apple APIs, etc.

I'm a JVM developer.

The mas update / mas outdated PR was the first time I coded in Swift or used Xcode.

I know nothing about standard Apple project structures, SPMs, Frameworks, etc., so I don't have the requisite knowledge to review this.

I'd happily review it, if I were qualified.

I can, however, tinker around to investigate the few issues I've run into, and try to fix them, when it's more of straight ahead programming rather than knowing the ins & outs of the whole Apple ecosystem / platform.

Sorry. Wish I could be more help.

@phatblat
Copy link
Member

phatblat commented May 2, 2021

No worries, @rgoldberg! Any help is very appreciated! You are welcome to propose changes even if you're not sure if the code is right. @chris-araman and I can help with suggestions. Or, we work out the Swift and lower level stuff with your input on testing and reproducing issues.

@phatblat phatblat self-assigned this May 2, 2021
@chris-araman
Copy link
Contributor Author

chris-araman commented May 2, 2021

One piece that I haven't figured out yet is what to do about the hosting for the bottle files for our custom homebrew-tap since Bintray has be sunsetted. This shouldn't block the release.

It looks like Homebrew has migrated to GitHub Packages. Perhaps we should too?

I've opened mas-cli/homebrew-tap#19 for tracking.

@phatblat
Copy link
Member

phatblat commented May 2, 2021

It looks like Homebrew has migrated to GitHub Packages. Perhaps we should too?

I've opened mas-cli/homebrew-tap#19 for tracking.

👍🏻 My thoughts exactly 😉

@phatblat
Copy link
Member

phatblat commented May 2, 2021

@chris-araman I'm re-reviewing this PR so we can merge today.

Copy link
Member

@phatblat phatblat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fantastic work! I love seeing deletes since they are removing cruft.

I found one issue with the version command that I'd like you to look into.

Sources/MasKit/Commands/Version.swift Show resolved Hide resolved
@phatblat phatblat merged commit 9277a8b into master May 3, 2021
@phatblat phatblat deleted the spm branch May 3, 2021 07:19
@phatblat phatblat added this to the 1.8.2 milestone Oct 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow running mas as portable or installing it
4 participants